test: add env name into test, and replace comons.fixtureDir with comm.fixtures #15816

shaohui-liu2000 · 2017-10-06T16:46:30Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows [commit guidelines]
(https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#commit-message-guidelines)

Affected core subsystem(s)

test

pawelgolda · 2017-10-06T18:02:40Z

test/parallel/test-http2-compat-serverrequest-pipe.js

@@ -10,7 +11,7 @@ const path = require('path');

 // piping should work as expected with createWriteStream

-const loc = path.join(common.fixturesDir, 'person.jpg');
+const loc = path.join(fixtures.fixturesDir, 'person.jpg');


This could be using method fixtures.path instead of path.join.

pawelgolda · 2017-10-06T18:56:44Z

test/parallel/test-http2-compat-serverrequest-pipe.js

@@ -10,8 +11,8 @@ const path = require('path');

 // piping should work as expected with createWriteStream

-const loc = path.join(common.fixturesDir, 'person.jpg');
-const fn = path.join(common.tmpDir, 'http2pipe.jpg');
+const loc = fixtures.path(fixtures.fixturesDir, 'person.jpg');


Please have a look at fixtures.path definition you will see that you don't have to pass the fixturesDir anymore.

pawelgolda · 2017-10-06T18:57:23Z

test/parallel/test-http2-compat-serverrequest-pipe.js

-const loc = path.join(common.fixturesDir, 'person.jpg');
-const fn = path.join(common.tmpDir, 'http2pipe.jpg');
+const loc = fixtures.path(fixtures.fixturesDir, 'person.jpg');
+const fn = fixtures.path(common.tmpDir, 'http2pipe.jpg');


This should be unchanged as it's not related to fixtures.

oops

shaohui-liu2000 · 2017-10-06T19:04:21Z

@pawelgolda PTAL

pawelgolda · 2017-10-06T19:10:20Z

@shaohui-liu2000 looks good.

rmg · 2017-10-06T19:31:37Z

test/parallel/test-child-process-default-options.js

@@ -45,5 +45,5 @@ child.stdout.on('data', function(chunk) {

 process.on('exit', function() {
  assert.ok(response.includes('HELLO=WORLD'),
-            'spawn did not use process.env as default');
+            `spawn did not use process.env as default(process.env.HELLO = ${process.env.HELLO})`);


The linter doesn't like this long line :-(

It can be fixed by shortening the line a bit or by adding an eslint directive to the top of the file (near 'use strict';) with something like /* eslint-disable max-len */ (you can see examples of this being done in other tests, such as test/parallel/test-readline-keys.js and test/parallel/test-require-extensions-same-filename-as-dir-trailing-slash.js.

You could disable the rule, but you shouldn't ;-). The line needs wrapping. This can be seen locally by calling make eslint.

Can you change this line to something like:

'spawn did not use process.env as default' + `(process.env.HELLO = ${process.env.HELLO})`);

so we don't have to disable the eslint rule?

lpinca · 2017-10-09T07:32:21Z

test/parallel/test-http2-compat-serverrequest-pipe.js

@@ -1,6 +1,7 @@
 'use strict';

 const common = require('../common');
+const fixtures = require('../common/fixtures');


Nit: can you please move this after the common.hasCrypto check?

Uh..now after the update there are two fixtures. I don't think it can run with two const fixtures defined? Can you remove this line?

joyeecheung · 2017-10-12T04:19:43Z

This should be landed as two commits.

CI: https://ci.nodejs.org/job/node-test-pull-request/10635/

gireeshpunathil · 2017-10-13T04:59:58Z

@sam-github @rmg @lpinca - PTAL.

PR-URL: #15816 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2017-10-13T20:03:22Z

Landed in 28e6cab and 7245e85

PR-URL: nodejs/node#15816 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #15816 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017

shaohui-liu2000 changed the title ~~add env name into test~~ test: add env name into test Oct 6, 2017

shaohui-liu2000 changed the title ~~test: add env name into test~~ test: add env name into test, and replace comons.fixtureDir with comm.fixtures Oct 6, 2017

pawelgolda reviewed Oct 6, 2017

View reviewed changes

shaohui-liu added 2 commits October 6, 2017 11:50

test: add env to failure message

f255996

test: replace common.fixtureDir with fixtures

2316e66

shaohui-liu2000 force-pushed the master branch from 4bb4807 to 26f3492 Compare October 6, 2017 18:53

sam-github previously approved these changes Oct 6, 2017

View reviewed changes

pawelgolda reviewed Oct 6, 2017

View reviewed changes

test: use fixtures.path to replace path.join

0c38a4e

shaohui-liu2000 force-pushed the master branch from 26f3492 to 0c38a4e Compare October 6, 2017 19:03

rmg reviewed Oct 6, 2017

View reviewed changes

fixup! test: use fixtures.path to replace path.join

ff9ae0f

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017

mscdex added child_process Issues and PRs related to the child_process subsystem. http2 Issues or PRs related to the http2 subsystem. labels Oct 6, 2017

lpinca reviewed Oct 9, 2017

View reviewed changes

shaohui-liu added 3 commits October 10, 2017 19:44

test:move fixture after common.hasCrypto check

6c7a88d

test:remove maxlen hint and split into 2 lines

fe11f68

remove extra fixture

60eccfe

joyeecheung approved these changes Oct 12, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 13, 2017

View reviewed changes

lpinca approved these changes Oct 13, 2017

View reviewed changes

jasnell approved these changes Oct 13, 2017

View reviewed changes

jasnell closed this Oct 13, 2017

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add env name into test, and replace comons.fixtureDir with comm.fixtures #15816

test: add env name into test, and replace comons.fixtureDir with comm.fixtures #15816

shaohui-liu2000 commented Oct 6, 2017 •

edited by sam-github

Loading

pawelgolda Oct 6, 2017

pawelgolda Oct 6, 2017

pawelgolda Oct 6, 2017

shaohui-liu2000 commented Oct 6, 2017

pawelgolda commented Oct 6, 2017

rmg Oct 6, 2017

sam-github Oct 7, 2017

joyeecheung Oct 11, 2017

lpinca Oct 9, 2017

joyeecheung Oct 12, 2017

joyeecheung commented Oct 12, 2017

gireeshpunathil commented Oct 13, 2017

jasnell commented Oct 13, 2017

test: add env name into test, and replace comons.fixtureDir with comm.fixtures #15816

test: add env name into test, and replace comons.fixtureDir with comm.fixtures #15816

Conversation

shaohui-liu2000 commented Oct 6, 2017 • edited by sam-github Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaohui-liu2000 commented Oct 6, 2017

pawelgolda commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Oct 12, 2017

gireeshpunathil commented Oct 13, 2017

jasnell commented Oct 13, 2017

shaohui-liu2000 commented Oct 6, 2017 •

edited by sam-github

Loading